fix(adapters): escape YAML frontmatter values consistently across all command adapters - #1447
Conversation
… command adapters
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesShared YAML serialization now consistently quotes and escapes frontmatter values, including tag arrays. Command-generation adapters use the shared helpers, with tests covering YAML round trips. Archive workflows now require inline synchronization and main-spec verification before archiving. YAML frontmatter generation
Archive workflow synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/core/command-generation/adapters.test.ts`:
- Around line 18-21: Update the YAML adapter matrix in the adapter tests to
include kiroAdapter and lingmaAdapter alongside the existing YAML-emitting
adapters. Keep kilocodeAdapter excluded from yamlAdapters because its dedicated
coverage verifies that it does not emit YAML frontmatter.
- Around line 977-997: Update escapeYamlValue, used by the adapter formatFile
flow, to quote values that YAML would interpret as non-string scalars, including
true, null, and numeric values, and to exclude indicator-prefixed text such as
“- starts here” from plain scalars. Extend the existing YAML frontmatter tests
with round-trip cases asserting these values parse back as the original strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b1b1ec2-de80-4b21-8370-9916276171d4
📒 Files selected for processing (23)
src/core/command-generation/adapters/amazon-q.tssrc/core/command-generation/adapters/antigravity.tssrc/core/command-generation/adapters/auggie.tssrc/core/command-generation/adapters/bob.tssrc/core/command-generation/adapters/claude.tssrc/core/command-generation/adapters/codebuddy.tssrc/core/command-generation/adapters/continue.tssrc/core/command-generation/adapters/costrict.tssrc/core/command-generation/adapters/crush.tssrc/core/command-generation/adapters/factory.tssrc/core/command-generation/adapters/github-copilot.tssrc/core/command-generation/adapters/iflow.tssrc/core/command-generation/adapters/junie.tssrc/core/command-generation/adapters/kiro.tssrc/core/command-generation/adapters/lingma.tssrc/core/command-generation/adapters/opencode.tssrc/core/command-generation/adapters/qoder.tssrc/core/command-generation/adapters/qwen.tssrc/core/command-generation/adapters/trae.tssrc/core/command-generation/adapters/windsurf.tssrc/core/command-generation/adapters/zcode.tssrc/core/command-generation/yaml.tstest/core/command-generation/adapters.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The affected encoder code is unchanged from the reproduced hold: values beginning with a block indicator can produce invalid YAML, while true, null, and numeric-looking strings round-trip as other scalar types. Please serialize every frontmatter string safely and add table-driven round-trip coverage across the changed adapters, including Kiro and Lingma.
Code review — verdict: good to mergeI was initially wary of this one — 23 files touching 20 adapters is exactly the shape that quietly changes output for every user. So I didn't review the diff; I generated the full artifact tree for all 21 affected tools on 1. Real need — yes, and it's bigger than the description suggests
---
name: OPSX: Apply
description: "Implement tasks from an OpenSpec change (Experimental)"
argument-hint: "[command arguments]"
---
Swept across every generated frontmatter file in the tree: So every command file OpenSpec generates for CodeBuddy, Crush, Lingma, and Qoder has broken frontmatter today. That's a shipping bug affecting four integrations, and it's invisible unless you actually parse the output — which is presumably why it survived this long. Good find. 2. Does it work — yes, all 24 fixed, nothing else broken0 unparseable files after. And critically, the fix doesn't just make things parse — it makes them parse to the right values. 3. Breaking changes — none semantically, and I verified this rather than assumingThis was my main concern, because the diff also removes quotes in places (several adapters previously always-quoted Zero semantic changes across 222 files, and zero body changes. Every quoting difference is stylistic —
The one thing to be aware of: on the next 4. Scope — correctly surgical, and net-negative complexity
On the testsFull suite on this branch: 112 files / 2275 tests, all passing, matching your reported numbers. For transparency: my first run showed 3 failures, but they did not reproduce on a clean re-run (that run took 332s vs. 45s, so it was almost certainly resource contention on my machine from parallel worktree builds, not this PR). I'm calling it green. The one modified existing expectation is correct and worth stating explicitly: - expect(output).toContain('description: \n');
+ expect(output).toContain('description: ""');That's not a weakened test — a bare Nit (optional, no action needed)
Harmless and still correct, but it's the source of most of the cosmetic churn above. Narrowing the regex would reduce the diff users see. That regex is pre-existing on Nice work — this is a well-targeted fix to a genuinely broken output path. |
97cbe7b to
2b63f0f
Compare
…nd table-driven adapter coverage
|
Updated YAML frontmatter serialization so all string values are safely double-quoted and preserve their type across edge cases. Added round-trip coverage for all 24 YAML adapters, including dedicated Kiro and Lingma tests. All 960 command-generation tests pass. |
…nous, and verified
|
Addressed review feedback in commit |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/command-generation/adapters.test.ts (1)
897-905: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test rationale: "quoting is triggered by another special char" no longer applies.
This test's title/comment describes the old conditional-quoting behavior (backslash alone didn't trigger quoting, only
:did). SinceescapeYamlValuenow always double-quotes and escapes every value unconditionally, the premise is outdated and may confuse future readers about why this assertion holds.📝 Suggested comment update
- it('should escape backslashes when quoting is triggered by another special char', () => { - // Backslash alone does not trigger quoting, but once quoting is on (via ':') - // every backslash must be doubled. Locks the replace(/\\/g, '\\\\') branch. + it('should escape backslashes in description', () => { + // Every value is now unconditionally double-quoted, so backslashes must + // always be doubled. Locks the replace(/\\/g, '\\\\') branch. const output = zcodeAdapter.formatFile({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/command-generation/adapters.test.ts` around lines 897 - 905, Update the test title and inline comments in the backslash-escaping case within the relevant adapter test to reflect that values are now always double-quoted and backslashes are escaped unconditionally. Keep the existing input and assertion unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/core/command-generation/adapters.test.ts`:
- Around line 897-905: Update the test title and inline comments in the
backslash-escaping case within the relevant adapter test to reflect that values
are now always double-quoted and backslashes are escaped unconditionally. Keep
the existing input and assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 14af19e8-fa20-4755-ba9e-7bed75fba36d
📒 Files selected for processing (9)
src/core/command-generation/adapters/continue.tssrc/core/command-generation/adapters/cursor.tssrc/core/command-generation/adapters/iflow.tssrc/core/command-generation/yaml.tstest/commands/artifact-workflow.test.tstest/core/command-generation/adapters.test.tstest/core/command-generation/generator.test.tstest/core/command-generation/yaml.test.tstest/core/init.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/command-generation/adapters/continue.ts
- src/core/command-generation/adapters/iflow.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/openspec-bulk-archive-change/SKILL.md`:
- Around line 132-146: Honor recorded conflict-resolution exclusions by making
delta-spec sync and verification conditional: route excluded, unimplemented
deltas through an explicit “sync skipped” path and allow their archive without
requiring verification, while retaining sync and verification for included
deltas. Apply this in skills/openspec-bulk-archive-change/SKILL.md (132-146),
both corresponding sections of
src/core/templates/workflows/bulk-archive-change.ts (134-146 and 405-417), and
update test/core/templates/skill-templates-parity.test.ts (253-277) to assert
the rendered instructions preserve the skipped-sync path and omit verification
requirements for excluded deltas.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac99c1df-b10d-4443-a904-b41d07d59e88
📒 Files selected for processing (4)
skills/openspec-bulk-archive-change/SKILL.mdsrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tstest/core/templates/skill-templates-parity.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The YAML hold is resolved at 206522d: all 24 YAML adapters now round-trip the scalar edge cases as strings, root-aware inline sync/verification is covered, and build, lint, focused tests, full tests, and generated-skill parity are clean. One blocker remains in bulk archive: step 5 can exclude an unimplemented delta from sync, but step 8 still syncs and verifies every delta unconditionally; please carry the recorded inclusion/exclusion set into execution, sync and verify only included deltas, explicitly report excluded ones as “sync skipped,” and cover the mixed included/excluded path in both generated variants.
|
Addressed the remaining bulk-archive blocker in b3b037b. Both generated variants now carry the per-delta inclusion/exclusion decisions into execution. Only included deltas are synced and verified; excluded deltas are reported explicitly as sync skipped. Added mixed included/excluded regression coverage for both generated variants. |
Resolve conflicts between this branch's bulk-archive rework and main's runtime-context/artifact-rules guidance (Fission-AI#1062) by unioning both sides: - archive-change: keep main's specs-instruction fetch gate; keep this branch's /opsx:sync naming in the opsx command variant. - bulk-archive: step 8a syncs only includedDeltas AND carries main's specs-rule snapshot + artifact-rule scoping bullets; guidelines lists keep both sides' bullets. - Regenerated skills/ and parity hashes rather than hand-merging them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Resolved the conflicts with What conflicted: only the archive/bulk-archive templates and their parity hashes. How it was resolved — union, nothing dropped from either side:
Verified: no |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at 62c7eab. The YAML serializers round-trip edge-case strings across all adapters, and bulk archive now carries per-delta included/excluded decisions through inline sync, verification, and sync-skipped reporting in both generated variants while preserving current runtime guidance. 1,094 focused tests, build, lint, generated-skill parity, the cross-platform suite, audit, dependency review, and release tracking all pass.
… close test gaps Adversarial review of the merged branch turned up four gaps: - Bulk archive tells the sync workflow to ignore `excludedDeltas`, but main's sync-specs calls `existingOutputPaths` the "complete list" of delta specs. An agent following both would sync the delta the caller withheld, step 8b would not catch it (it verifies only included deltas), and the run would still report `sync skipped`. Sync now honors a caller-supplied subset, mirroring the inline rule-snapshot handoff main already added. - escapeYamlValue left C0/DEL/C1 control characters raw. The repo's own parser accepts them, so tests passed while stricter parsers used by other tools reject the document. Emit them as \xHH. - The adapter matrix was a hand-maintained list driving only `description`, so raw interpolation in lingma's name/category/tags — and any newly registered adapter — passed green. It now derives from the registry and drives every string field. - Four bulk-archive template lines were guarded only by golden hashes, which this repo regenerates as routine. Also corrects the escapeYamlValue docstring, which still described the pre-PR conditional-quoting behavior, and adds the missing changeset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Ran four adversarial review passes over the merged branch (YAML correctness, template integrity, mutation testing, downstream blast radius). Fixes pushed in 62bb846. One real functional defect, introduced by the merge. Step 8a tells the sync workflow to sync only Escaping hardened one class further. Two test gaps that mattered:
Also corrected the Left alone deliberately, as pre-existing and outside this PR:
Full suite 3113 passing, lint and |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at 62bb846. The latest fixes correctly preserve caller-supplied delta subsets in both sync variants, escape C0/DEL/C1 control characters for strict YAML parsers, and derive adapter/string-field coverage from the registry. Build, lint, 1,112 focused tests, strict spec validation, generated-skill parity, cross-platform CI, audit, dependency review, and release tracking all pass.
A second adversarial pass found the previous fix incomplete. Narrowing
step 3 ("Find delta specs") left step 4 — the loop that actually applies
the changes — still reading "for each capability delta spec path
returned by the CLI". An agent treating step 3 as descriptive and step 4
as operative re-widens to the full list and syncs the delta bulk archive
withheld: the original defect, one step further down the template.
Step 4 now iterates the step-3 selection, and the parity test pins both
the new wording and the absence of the old.
Also from that pass:
- Generalize the carve-out beyond archive. It was conditioned on
"archive invoked this workflow inline", so a user asking /opsx:sync to
sync one delta read as an instruction to ignore them.
- Define the two undefined edges: a named path outside
existingOutputPaths, and an empty named list. Both stop and report
rather than proceeding on a guess.
- Drive control characters through the adapter matrix. It drove none, so
the escaping this suite exists to prove had no adapter-level coverage
and the raw-CR assertion could never fail. Verified live by mutation.
- Give contentDerivedFields two markers that differ in length and shape.
Same-shaped markers render identically for a length- or slice-derived
field, which would drop it from every assertion silently.
Drops the `not.toContain('complete list of delta spec files')`
assertion: it banned one exact synonym while any reword of the same
conflicting instruction passed, so it read as coverage without being it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Second adversarial round, on the previous round's own fixes rather than the original diff. Pushed as 5bfd574. The earlier fix was incomplete, in the same way and for the same reason. Narrowing step 3 ("Find delta specs") and the guardrails left step 4 — the loop that actually applies the changes — still reading "For each capability delta spec path returned by the CLI." An agent that reads step 3 as descriptive and step 4 as operative re-widens to the full list and syncs the delta bulk archive withheld. Same wrong outcome as the original defect, one step further down the template. Step 4 now iterates the step-3 selection, and the parity test pins both the new wording and the absence of the old, verified by mutation. Two more from that pass:
The control-character escaping was independently validated against js-yaml, PyYAML and the repo's Test hardening, both found by mutating the previous round's tests:
Exhaustive mutation of the rebuilt matrix — every adapter × every content-derived field × all cases — found 0 survivors, plus 14 in-tree mutations all killed. I also dropped the Non-blocking, not fixed here: sync's success output has no place to list withheld paths, so a caller can't cross-check what was skipped; and 3257 tests passing, lint and |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at 5bfd574. The sync workflow now carries the step-3 delta selection into the operative step-4 loop, stops on out-of-scope or empty named subsets, and supports caller narrowing beyond archive without widening back to the CLI list. The adapter matrix now exercises control characters and uses shape/length-distinct markers. Build, lint, 1,256 focused tests, strict spec validation, generated-skill parity, cross-platform CI, audit, dependency review, and release tracking all pass.
…t skills Addresses the two release blockers on this PR. Archive: the change's MODIFIED blocks were written against an older canonical `cli-init`, so `openspec archive add-devin-desktop-support` aborted rather than merging. The deltas are regenerated from the current canonical specs (cli-init `Skill Generation` + `Slash Command Generation`, cli-update `Slash Command Updates`, and a new `ai-tool-paths` delta for the `.devin` skillsDir), each restating every existing scenario so archive is purely additive. Invocation syntax: only Devin Desktop reads `.devin/workflows/`, so a `/opsx-*` workflow reference is dead text on Devin Local, which supports skills only. Devin now takes the skill-reference transformer, so skill bodies and the getting-started hint say `/openspec-*`. Workflow bodies keep hyphen references, applied by devinAdapter itself. The adapter also drops its private copy of escapeYamlValue / formatTagsArray in favor of the shared helpers main centralized in Fission-AI#1447, which quote unconditionally and escape control characters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…AI#1167) * proposal: add devin desktop support * feat(adapters): add devin desktop command adapter - Create new Devin Desktop adapter for .devin/workflows/opsx-<id>.md - Register adapter in CommandAdapterRegistry - Export adapter from adapters index - Update docs/supported-tools.md with Devin Desktop entry - Add 'devin' to available tool IDs list Devin Desktop uses the same Cascade workflow system as Windsurf, making it a natural migration path for existing users. * fix(config): add devin desktop to AI_TOOLS Add Devin Desktop entry to AI_TOOLS configuration so that: - getToolsWithSkillsDir() includes 'devin' as a valid tool ID - getWorkspaceSkillToolIds() returns 'devin' in the list - parseWorkspaceSkillToolsValue() accepts 'devin' as valid input - openspec init --tools devin works correctly This fixes validation failures where 'devin' was documented in docs/supported-tools.md but not recognized by validation functions that derive valid IDs from AI_TOOLS. * fix(devin-adapter): escape implicit YAML scalars in frontmatter Update escapeYamlValue to detect and quote implicit YAML scalars that would be coerced by parsers: - Booleans: true, false, yes, no, on, off - Null variants: null, ~ - Numbers: integers, floats, exponentials, hex (0x), octal (0o) - Edge cases: standalone dash (-) and dot (.) This ensures values like 'true', '123', 'null' remain strings in YAML frontmatter instead of being interpreted as booleans, numbers, or nulls. Preserves existing escaping logic for special characters and newlines. * test(devin-adapter): add comprehensive tests for Devin Desktop adapter Add test coverage for the Devin Desktop adapter including: - Command reference transformation from colon to hyphen syntax - YAML frontmatter escaping for special characters and implicit scalars - File path generation for workflows - Integration with available tools detection - Init and update command workflows * Add cross-platform testcase. * fix(devin): refresh deltas against canonical specs and point skills at skills Addresses the two release blockers on this PR. Archive: the change's MODIFIED blocks were written against an older canonical `cli-init`, so `openspec archive add-devin-desktop-support` aborted rather than merging. The deltas are regenerated from the current canonical specs (cli-init `Skill Generation` + `Slash Command Generation`, cli-update `Slash Command Updates`, and a new `ai-tool-paths` delta for the `.devin` skillsDir), each restating every existing scenario so archive is purely additive. Invocation syntax: only Devin Desktop reads `.devin/workflows/`, so a `/opsx-*` workflow reference is dead text on Devin Local, which supports skills only. Devin now takes the skill-reference transformer, so skill bodies and the getting-started hint say `/openspec-*`. Workflow bodies keep hyphen references, applied by devinAdapter itself. The adapter also drops its private copy of escapeYamlValue / formatTagsArray in favor of the shared helpers main centralized in Fission-AI#1447, which quote unconditionally and escape control characters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(devin): correct commands-only hint, fill doc gaps, cover both surfaces Follow-up from adversarial review of the previous commit. The devin special case in getTransformerForTool was unconditional, so under commands-only delivery — where `.devin/skills/` is deleted — the getting-started hint named `/openspec-propose`, a skill that is not on disk. Devin now takes the skill transformer only when skills are generated, and the hyphen form otherwise. The cli-init delta records the fallback, and a unit test pins all three delivery modes. Docs: `devin` was missing from the `--tools` list in docs/cli.md (which mirrors the list supported-tools.md already had) and from the command-syntax tables in docs/commands.md and docs/how-commands-work.md. The supported-tools row gains a footnote citing Cognition's docs for the `.windsurf/` -> `.devin/` move and the Devin Local workflow gap. Tests: init and update now assert both surfaces — workflows carry `/opsx-*`, skills carry `/openspec-*`, neither carries `/opsx:` — and update checks the seeded skill was actually refreshed. Adds the negative detection case. Drops three devin-only YAML assertions that duplicated, less rigorously, the registry-derived escaping matrix that now enrolls devin automatically. Also reverts an unrelated zcode export and lingma reorder that a merge resolution had pulled into adapters/index.ts. zcodeAdapter is registered but missing from that barrel on main; that is a pre-existing gap and belongs in its own change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(devin): name the right command in the profile migration notice The profile-migration notice printed by both `init` and `update` hardcoded `/opsx:propose` for every adapter-backed tool. Devin registers no such command on any surface — its workflows answer to `/opsx-propose` and its skills to `/openspec-propose` — so an upgrading Devin user was told to run something that does not exist: Migrated: custom profile with 6 workflows New in this version: /opsx:propose. The reference now goes through getTransformerForTool, the same call init.ts already makes for the getting-started hint. Devin prints `/openspec-propose`; opencode and the other filename-invoked tools are corrected to `/opsx-propose` as a side effect; claude is unchanged. Also corrects two inherited false claims in the cli-update delta — Devin workflows carry no OpenSpec markers, and update writes every profile workflow rather than only refreshing files that already exist, which the PR's own test demonstrates. Qualifies the supported-tools footnote for commands-only delivery, and strips trailing whitespace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(devin): keep the cli-update delta in step with the canonical spec The delta restates the whole 'Slash Command Updates' requirement, and its copy of the OpenCode scenario predated Fission-AI#1471 — archiving it would have quietly reverted the spec to calling the hyphen rewrite an OpenCode special case, the hand-maintained framing Fission-AI#1471 removed. Archive on a scratch copy is now purely additive. Also point tasks.md at the generator rather than the deleted transformToHyphenCommands, and enroll devin in the pure-formatter tripwire — it is the one adapter whose private body transform was just removed, so it is the likeliest to have it re-added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(adapters): follow the Windsurf rename to Devin Desktop, with migration Windsurf was rebranded to Devin Desktop on 2026-06-02 and its config directory moved: `.devin/` is the preferred read+write location, `.windsurf/` a legacy read-only fallback. Devin Local does not read `.windsurf/` at all, so an existing Windsurf user's OpenSpec files are invisible to it. Carrying `devin` as a second tool id alongside `windsurf` would list one product twice and leave upgraders with two parallel installs — `openspec update` even told them to create the second one ("Detected new tool: Devin Desktop"). This follows the rename instead, as the repo already did for Kimi CLI -> Kimi Code: - `windsurf` is retired as a tool id; `devin` takes its place, with `detectionPaths: ['.devin', '.windsurf']` so pre-rebrand projects are still recognized. The Windsurf adapter is replaced, not duplicated. - `TOOL_ID_ALIASES` keeps `--tools windsurf` resolving, so existing setup scripts and CI keep working; they now configure `.devin/`. - OpenSpec-managed skills (`openspec-*`) and command files (`opsx-*`) under `.windsurf/` move to `.devin/`. The kimi migration handled skills only; command files now move too, deriving the legacy path from the adapter's own getFilePath rather than hard-coding a layout. - The move is offered, not taken: nothing on disk distinguishes a user who took the rebrand from one still on a pre-rebrand Windsurf build that reads only `.windsurf/`. `openspec update` explains the rename and asks; --force and non-interactive runs migrate; declining leaves every file untouched and says what that costs. Files the user wrote are never moved. Also gives Devin its own row in the authoritative invocation table — the catch-all row claimed `/opsx-<id>` for both agents, which is wrong for Devin Local. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(devin): stop the migration from deleting anything it does not own An adversarial pass found two ways the move destroyed files. Symlinked roots wiped the install. `ln -s .devin .windsurf` is a realistic way to straddle the rebrand, and it makes source and destination the same file — so the "destination exists, drop the legacy copy" branch deleted the only copy. Twelve generated files, gone, and not regenerated: the wipe happens before tool detection, so update then reported no configured tools. Both roots are now realpath'd and a self-move is skipped. User content inside an OpenSpec-managed path was deleted. The same branch rm -rf'd the whole legacy skill directory, taking a hand-written reference.md beside SKILL.md with it, and deleted a legacy command file even when the user had edited it. Now only SKILL.md is removed from a skill directory, and a command file is removed only when byte-identical to the one that survives — an edit is left where it is. Also: declining the move stranded the user. `update` then printed "No configured tools found. Run openspec init", which is wrong — the project is configured, just in the directory OpenSpec no longer writes. It now says so and how to resume. A closed stdin during the prompt aborted the whole update; it is treated as a decline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(devin): add a changeset for the Windsurf rename and migration Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(devin): move only SKILL.md, never the skill directory around it alfred caught a data-loss path the earlier fix missed. When the destination did not yet exist, migration renamed the whole legacy skill directory into `.devin/` — carrying any file the user kept beside `SKILL.md` with it. That destination is a directory OpenSpec owns and removes on its own: under commands-only delivery, or for a workflow outside the active profile. So the move handed the user's file to a later rm and it vanished. Reproduced on `d94af8b`: with `delivery: commands`, a `reference.md` beside a legacy `SKILL.md` was gone after `openspec update`. Only `SKILL.md` crosses now, in both branches; anything else stays under the legacy root, and the legacy directory is still removed when the move leaves it empty. Regression tests cover the commands-only and deselected-workflow cases and both fail against the previous code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(devin): treat an edited skill the way an edited command is already treated A final adversarial pass found the two paths disagreeing. When both roots held the same file with different content, the command path compared bytes and kept the user's version; the skill path deleted it with no comparison — so one `openspec update` destroyed an edited SKILL.md while preserving an edited opsx-*.md in the same project. Both now share one `classifyManagedFile` rule: move when the destination is empty, drop the legacy copy only when byte-identical, otherwise leave it. Anything left behind is reported, so a user who customized a file knows two copies exist rather than discovering it later. Note on the other finding from that pass: OpenSpec regenerating or pruning the files it owns is long-standing behavior, not something this PR introduces. Verified against main — an edited SKILL.md under a deselected workflow, and an edited selected skill and command, are all destroyed by `openspec update` on 9a937cb too. No regression, so left alone here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(devin): report divergent legacy files even when nothing is movable collectLegacyToolMigrations only returned a result when something moved, so a project where EVERY legacy file differs from its counterpart produced no output at all — two divergent copies and not a word about them. That is the one case where the report matters most, since it is entirely made of files the migration deliberately refused to touch. Kept-only results are retained now. Callers gate on hasMovableContent(), so a kept-only result reports what was left without offering to move nothing and without claiming a migration that did not happen. Also reworded the notice. A legacy file can differ because the user edited it or simply because an older OpenSpec generated it, so it no longer asserts an edit — it states that nothing was overwritten and leaves the user to compare the two copies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(devin): stop matching the unrelated profile-migration line The kept-only regression asserted no line matched /Migrated\s*:/, which also matches OpenSpec's profile migration message, "Migrated: custom profile with N workflows". That line only prints when the global config has no profile yet — true on a fresh CI runner, false on a developer machine that has run OpenSpec before — so the test passed locally and failed on all three CI platforms. Now matched on the directory arrow, ".windsurf → .devin", which is specific to a migration report and unaffected by config state. Reproduced both ways with an empty XDG_CONFIG_HOME: the old assertion fails there, the new one passes, and the full suite is green under CI's XDG_CONFIG_HOME + VITEST_MAX_WORKERS=4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Clay Good <hi@claygood.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gemini adapter interpolated the description into a TOML basic string and the body into a multiline basic string with no escaping. Every current template value happens to be safe; the first description with a double quote or backslash would silently produce invalid TOML for all Gemini command files. Escape both contexts (#1447 fixed the same class for the YAML adapters but scoped itself to YAML). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-audit follow-ups (Fission-AI#1475) * fix(archive): make the scenario-drift check fence-aware parseScenarioBlocks matched #### Scenario: headers on raw lines while the validator's countScenarios masks fenced code blocks (Fission-AI#1151). The drift check (Fission-AI#1391) inherited the raw scan, so a fenced scenario example in the current spec aborted an archive that validate had passed, and a fenced name in the MODIFIED block counted as keeping a scenario the block had actually dropped. Build the shared code-fence mask and skip masked lines in both the header scan and the block-end scan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): tear down the redirected request when the budget expires The overall request budget was armed inside the first send() and its callback closed over that hop's request. After a redirect the timer destroyed the already-dead first request, so a redirect target that trickled bytes kept resetting its idle timeout and held the socket open until the body-size cap. Track the in-flight request and have the budget timer destroy whichever one is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(release): add changesets for user-facing changes missing from the 1.7.0 notes 18 feat/fix commits merged since v1.6.0 without a changeset, so the pending Version Packages PR would have released them silently: five tool integrations (ZCode, Hermes, CodeArts, Kimi Code rename, Codex skills-only), skills.sh distribution, symlinked schema dirs, nested spec discovery, drift multiplicity, checkbox markers, Windows welcome input, npx avoidance, doctor store drift, local dates, missing-core-workflows warning, store-aware main specs, open-questions guidance, and spec content guidance. Plus changesets for this branch's two fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(adapters): escape TOML-active characters in Gemini command files The gemini adapter interpolated the description into a TOML basic string and the body into a multiline basic string with no escaping. Every current template value happens to be safe; the first description with a double quote or backslash would silently produce invalid TOML for all Gemini command files. Escape both contexts (Fission-AI#1447 fixed the same class for the YAML adapters but scoped itself to YAML). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): harden install detection and redirect handling Three follow-ups from the release audit: - A path segment literally named volta (a user or project directory) classified the install as volta-managed and swallowed the upgrade offer. The undotted spelling now requires volta's own tools/image layout, matching how pnpm and yarn already demand corroboration. - The Windows npm-ownership fallback checked that the npm prefix exists, which is true of any X\node_modules\pkg tree, hand-copied ones included. Corroborate with the openspec.cmd shim npm actually writes. - A https registry redirecting to plain http was followed; a MITM on that reply controls the newer-version answer. Refuse the downgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(cli): export zcodeAdapter from the barrel and sync a completion description zcode was registered but missing from the adapters barrel (its test imported the module directly), and the completion registry still carried the pre-Fission-AI#1062 description for the instructions command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(parser): strip a UTF-8 BOM before parsing specs and deltas A BOM-prefixed delta spec (Windows editors, PowerShell Out-File) failed validate and archive with 'No delta sections found' because the first line never matched '## ADDED Requirements'. Strip the BOM in both normalizers, the same way tool detection already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): reject over-long change names with a validation message A 300-character change name surfaced two raw ENAMETOOLONG errno dumps from stat and mkdir. Bound the name at 200 characters in validateChangeName so the failure is a normal validation error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): finish the early-sync no-op rules for MODIFIED and RENAMED Two asymmetries left over from the Fission-AI#1376/Fission-AI#1386/Fission-AI#1437 no-op work: - MODIFIED counted every delta as applied even when the block was byte-equal to the main spec, so a fully early-synced change rewrote the file (normalization churn), printed '~ N modified', and reported specsUpdated: true where its ADDED/REMOVED/RENAMED twins print 'Specs already in sync; no files changed.' Count only real replacements. - RENAMED's already-synced skip (source gone, target present) had no near-miss guard: a case/whitespace variant of the source still in the spec means a typo'd header, and REMOVED already hard-aborts on that signal. Apply the same guard, excluding the target itself so a case-only rename still no-ops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(validate): stop reporting an unreadable specs dir as 'no deltas' The delta-validation loop swallowed every error as 'if no specs dir, treat as no deltas', so an EACCES capability folder produced the misleading 'Change must have at least one delta' while archive let the same error propagate. Tolerate only ENOENT and ENOTDIR (a stray specs file); anything else stays loud, matching discoverSpecFiles' documented fail-loud contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): say when commands-only delivery leaves a tool with nothing Under delivery: commands, update removed the skills of adapterless skills-only tools (Hermes, Kimi Code, Vibe, CodeArts, ForgeCode) without a word — leaving zero OpenSpec artifacts while the tool's detection dir kept re-suggesting an init that would also generate nothing. Print the same per-tool configuration correction init already prints, pointing at 'openspec config set delivery both'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(completion): honor $ZSH and $ZSH_CUSTOM for Oh My Zsh installs The installer used a set $ZSH only as an is-installed signal and then wrote to ~/.oh-my-zsh regardless, so a custom OMZ location got a freshly created ~/.oh-my-zsh tree that no shell ever loads — and isInstalled/uninstall looked in the same wrong place. Route every path through the $ZSH/$ZSH_CUSTOM-aware helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(init): make the static welcome screen wait for the Enter it asks for The static branch printed 'Press Enter to select tools...' and returned immediately, so the Enter landed in the tool picker and submitted the pre-selected set sight-unseen. Fission-AI#1462 routed reduced-motion, OPENSPEC_NO_ANIMATION, --no-animation, NO_COLOR, and narrow-terminal users onto this path. Wait in a TTY; drop the prompt line when there is no TTY to wait on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(feedback): keep the manual fallback on every gh failure Only missing-gh and unauthenticated flows showed the formatted feedback and pre-filled submission URL; issues-disabled, network, or rate-limit failures printed gh's stderr and discarded the path to submit what the user had already typed. Route those through the same manual fallback, preserving gh's exit code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(update): model the npm shim in the Windows prefix fixture The ownership corroboration now checks for the openspec.cmd shim npm writes beside node_modules; the Homebrew-prefix fixture built the layout without it, so the test failed on windows-pwsh. Write the shim in the fixture and pin the inverse: the same shape with nothing npm wrote (a hand-copied portable tree) is not an npm install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): require volta's full tools/image layout for the undotted spelling The corroboration used has('tools', 'image'), which is some() — volta AND (tools OR image) — so /srv/volta/tools/apps/... still classified as a Volta install and swallowed the upgrade offer. Require both segments, matching the real %LOCALAPPDATA%\Volta\tools\image layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(adapters): escape control characters in Gemini multiline prompts escapeTomlMultilineBasicString handled backslashes and quote-triples but not the C0 controls that are as invalid in a multiline basic string as in a single-line one. Reuse TOML_CONTROL_CHARS, applied last so the escapes it introduces are not re-doubled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(completion): finish the $ZSH_CUSTOM support and isolate it in tests The fpath verification advice still grepped the literal custom/completions, which a relocated $ZSH_CUSTOM need never contain — grep the actual directory instead. The installer tests cleared only $ZSH, so on a machine exporting $ZSH_CUSTOM they would have written into (and deleted from) the developer's real OMZ custom dir — the same leakage class Fission-AI#1400 fixed for $ZSH. Clear/restore both, and pin the custom-location paths with two new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(release): correct the hermes and zcode changeset wording Hermes is skills-only (no command adapter), and zcode's namespaced commands register /opsx:<id>, not /opsx-* — the release notes must not reintroduce the invocation-spelling confusion Fission-AI#1471 removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(feedback): pin the manual fallback on a non-label gh failure The new reportGhFailure output (formatted feedback + pre-filled URL) had no coverage; the network-failure test now asserts it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(completion): match fpath entries as literal strings in the OMZ guidance The verification advice interpolated the completions dir into grep "<dir>" where regex metacharacters make the check unreliable and quotes could break the displayed command. Print one fpath entry per line and match with grep -F on a shell-quoted literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(adapters): never emit a bare carriage return in Gemini TOML prompts A lone CR is illegal in a multiline basic string — Python 3.13 tomllib rejects the file — and the control-char pass deliberately skipped it on the assumption it only appears as CRLF. Normalize CRLF to LF and escape any remaining CR as \r. The escaping guarantee is now parser-backed: smol-toml (new devDependency) round-trips every hostile body in the regression matrix (lone CR, CRLF, CR before a quote run, NUL/VT/FF, trailing backslash, four- and five-quote runs), and the same outputs were verified against Python tomllib. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(nix): update the pnpm deps hash for the smol-toml devDependency The lockfile changed, so the fixed-output derivation hash moved; value taken from the CI mismatch report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Description
Centralize and enforce YAML frontmatter value escaping across all tool command adapters.
Root Cause
Command adapters emitting YAML frontmatter (
amazon-q,antigravity,auggie,bob,codebuddy,continue,costrict,crush,factory,github-copilot,iflow,junie,kiro,lingma,opencode,qoder,qwen,trae,zcode) either omitted string escaping or re-implemented local un-escaped/duplicate variants ofescapeYamlValue. When command descriptions, names, categories, or tags contained YAML special characters (e.g. colons:, quotes", newlines\n, carriage returns\r, or brackets[]), generated command files contained invalid YAML frontmatter that failed YAML parsers.Fix Details
escapeYamlValueandformatTagsArrayfromsrc/core/command-generation/yaml.tsas the single authoritative helper module.escapeYamlValueto return""when given an empty string so empty fields produce valid YAML scalars instead of unquoted trailing whitespace.amazon-q,antigravity,auggie,bob,codebuddy,continue,costrict,crush,factory,github-copilot,iflow,junie,kiro,lingma,opencode,qoder,qwen,trae,windsurf,zcode) to import and use sharedescapeYamlValueandformatTagsArrayhelpers.test/core/command-generation/adapters.test.tsverifying that frontmatter emitted across all YAML adapters parses cleanly withyaml.parse.Verification
pnpm test(112 test files passed, 2275 tests passed)pnpm run build(Clean build completion)pnpm run lint(0 ESLint errors)pnpm tsc --noEmit(0 type errors)git diff --check(Clean formatting, no trailing whitespace)Summary by CodeRabbit
Bug Fixes
""and tag frontmatter values are emitted in a consistent YAML-safe array format.Tests
Documentation